-
Notifications
You must be signed in to change notification settings - Fork 874
Support for ChatOptions.ResponseFormat in AWSSDK.Extensions.Bedrock.MEAI #4113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: development
Are you sure you want to change the base?
Support for ChatOptions.ResponseFormat in AWSSDK.Extensions.Bedrock.MEAI #4113
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds ResponseFormat support to the AWS Bedrock ChatClient for Microsoft.Extensions.AI, enabling structured JSON output from Bedrock models. The implementation uses Bedrock's tool mechanism with a synthetic tool to enforce structured responses, requiring models with ToolChoice support (Claude 3+ and Mistral Large).
Key changes:
- Implemented ResponseFormat handling via synthetic tool creation that forces models to return structured JSON
- Added error handling for unsupported models and missing structured responses
- Added Document-to-JSON conversion utilities for extracting structured content from tool use responses
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| extensions/src/AWSSDK.Extensions.Bedrock.MEAI/BedrockChatClient.cs | Core implementation of ResponseFormat support including synthetic tool creation, error handling for unsupported models, Document-to-JSON conversion, and validation that ResponseFormat conflicts with user-provided tools |
| extensions/test/BedrockMEAITests/BedrockChatClientTests.cs | Added MockBedrockRuntime test infrastructure and two tests validating schema conversion and JSON extraction from tool use responses |
extensions/src/AWSSDK.Extensions.Bedrock.MEAI/BedrockChatClient.cs
Outdated
Show resolved
Hide resolved
extensions/src/AWSSDK.Extensions.Bedrock.MEAI/BedrockChatClient.cs
Outdated
Show resolved
Hide resolved
GarrettBeatty
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you explain more a little more in the PR why we need make this synthetic tool and what not
| } | ||
|
|
||
| // Check if ResponseFormat is set - not supported for streaming yet | ||
| if (options?.ResponseFormat is ChatResponseFormatJson) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just wondering why this is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as discussed its because by supplying only one tool it forces bedrock to use our json one. please add comment explaining this and also link to other sdk with example
| } | ||
|
|
||
| /// <summary>Converts a <see cref="Document"/> to a JSON string.</summary> | ||
| private static string DocumentToJsonString(Document document) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all of this stuff i would be surprised if it doesnt exist already in a jsonutils or utils file. either way it shouldnt be in this class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with garret, it should live in a utils class at the least
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked at the entire PR, but we've had requests in the past to make the Document class interop better with JSON.
It's something we should do, but we have to be aware the document type is meant to be agnostic (the service could start returning CBOR tomorrow for example). See this comment from Norm: #3915 (comment)
It'd probably make more sense to include this functionality in Core, but now I'm even wondering if it's better to do that first (and separately) from this PR.
extensions/src/AWSSDK.Extensions.Bedrock.MEAI/BedrockChatClient.cs
Outdated
Show resolved
Hide resolved
peterrsongg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My long comment on testing different json responses returned by the service might not make sense depending on what this feature is supposed to do. So if this option is set, that tells Bedrock to return the response in a certain way?
I still think we shouldn't have a MockBedrockRuntime that implements IAmazonBedrockRuntime. We will have to update this class every time a new operation is released.
| } | ||
|
|
||
| /// <summary>Converts a <see cref="Document"/> to a JSON string.</summary> | ||
| private static string DocumentToJsonString(Document document) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with garret, it should live in a utils class at the least
| // Check if this is a ToolChoice validation error (model doesn't support it) | ||
| bool isToolChoiceNotSupported = | ||
| ex.ErrorCode == "ValidationException" && | ||
| (ex.Message.IndexOf("toolChoice", StringComparison.OrdinalIgnoreCase) >= 0 || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is checking the error message the only way to achieve this? error messages aren't gauranteed to stay the same.
| } | ||
|
|
||
| // Assert | ||
| var tool = mock.CapturedRequest.ToolConfig.Tools[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm a bit confused, and maybe it is because i don't understand who is supposed to return the response in the provided schema (bedrock or us), but this test case just seems to be asserting that the tool has the correct schema set on it. Is there no way to test the actual functionality?
…handling, restore docs
…ge per PR feedback
a459b62 to
b7bd419
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
extensions/test/BedrockMEAITests/BedrockMEAITests.NetFramework.csproj:37
- Missing corresponding NetStandard test project. The repository follows a pattern where extension tests have both NetFramework and NetStandard project files to ensure platform compatibility (see CloudFront.SignersTests and EC2.DecryptPasswordTests as examples). A NetStandard test project targeting
netcoreapp3.1;net8.0is needed to ensure tests run on .NET Core 3.1 and .NET 8.0, as required by the contributing guidelines.
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFrameworks>net472</TargetFrameworks>
<DefineConstants>$(DefineConstants);BCL</DefineConstants>
<AssemblyName>BedrockMEAITests</AssemblyName>
<PackageId>BedrockMEAITests</PackageId>
<GenerateAssemblyTitleAttribute>false</GenerateAssemblyTitleAttribute>
<GenerateAssemblyConfigurationAttribute>false</GenerateAssemblyConfigurationAttribute>
<GenerateAssemblyCompanyAttribute>false</GenerateAssemblyCompanyAttribute>
<GenerateAssemblyProductAttribute>false</GenerateAssemblyProductAttribute>
<GenerateAssemblyDescriptionAttribute>false</GenerateAssemblyDescriptionAttribute>
<GenerateAssemblyCopyrightAttribute>false</GenerateAssemblyCopyrightAttribute>
<GenerateAssemblyVersionAttribute>false</GenerateAssemblyVersionAttribute>
<GenerateAssemblyFileVersionAttribute>false</GenerateAssemblyFileVersionAttribute>
<TreatWarningsAsErrors>true</TreatWarningsAsErrors>
<LangVersion>Latest</LangVersion>
</PropertyGroup>
<ItemGroup>
<PackageReference Include="Microsoft.Extensions.AI.Abstractions" Version="9.9.1" />
<PackageReference Include="Moq" Version="4.8.3" />
<PackageReference Include="xunit" Version="2.9.2" />
<PackageReference Include="xunit.runner.visualstudio" Version="2.8.2" />
</ItemGroup>
<ItemGroup>
<ProjectReference Include="../../../sdk/src/Core/AWSSDK.Core.NetFramework.csproj" />
<ProjectReference Include="../../src/AWSSDK.Extensions.Bedrock.MEAI/AWSSDK.Extensions.Bedrock.MEAI.NetFramework.csproj" />
<ProjectReference Include="../../../sdk/test/UnitTests/Custom/AWSSDK.UnitTestUtilities.NetFramework.csproj" />
</ItemGroup>
<ItemGroup>
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.11.1" />
</ItemGroup>
</Project>
extensions/src/AWSSDK.Extensions.Bedrock.MEAI/BedrockChatClient.cs
Outdated
Show resolved
Hide resolved
extensions/src/AWSSDK.Extensions.Bedrock.MEAI/BedrockChatClient.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
peterrsongg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a few comments, but this looks much better with all the additional test cases i feel much more confident about this change. thanks for applying my earlier feedback
extensions/src/AWSSDK.Extensions.Bedrock.MEAI/BedrockChatClient.cs
Outdated
Show resolved
Hide resolved
extensions/src/AWSSDK.Extensions.Bedrock.MEAI/BedrockChatClient.cs
Outdated
Show resolved
Hide resolved
extensions/src/AWSSDK.Extensions.Bedrock.MEAI/BedrockChatClient.cs
Outdated
Show resolved
Hide resolved
extensions/src/AWSSDK.Extensions.Bedrock.MEAI/BedrockChatClient.cs
Outdated
Show resolved
Hide resolved
extensions/src/AWSSDK.Extensions.Bedrock.MEAI/BedrockChatClient.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
extensions/src/AWSSDK.Extensions.Bedrock.MEAI/BedrockChatClient.cs
Outdated
Show resolved
Hide resolved
extensions/test/BedrockMEAITests/BedrockMEAITests.NetFramework.csproj
Outdated
Show resolved
Hide resolved
extensions/src/AWSSDK.Extensions.Bedrock.MEAI/BedrockChatClient.cs
Outdated
Show resolved
Hide resolved
extensions/src/AWSSDK.Extensions.Bedrock.MEAI/BedrockChatClient.cs
Outdated
Show resolved
Hide resolved
| Role = ChatRole.Assistant, | ||
| MessageId = Guid.NewGuid().ToString("N"), | ||
| }; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public async Task<ChatResponse> GetResponseAsync(
IEnumerable<ChatMessage> messages, ChatOptions? options = null, CancellationToken cancellationToken = default)
{
if (messages is null)
{
throw new ArgumentNullException(nameof(messages));
}
// Validate ResponseFormat usage early
ValidateResponseFormatUsage(options, isStreaming: false);
ConverseRequest request = options?.RawRepresentationFactory?.Invoke(this) as ConverseRequest ?? new();
// ... rest of request setup ...
ConverseResponse response = await _runtime.ConverseAsync(request, cancellationToken).ConfigureAwait(false);
// Extract structured response if using ResponseFormat
if (options?.ResponseFormat is ChatResponseFormatJson)
{
return TryExtractStructuredResponse(response, request.ModelId, options);
}
// Normal content processing...
ChatMessage result = CreateChatMessageFromResponse(response, options);
return CreateChatResponse(result, response);
}
i dont want to add more work to this PR but im thinking eventually we need to refactor this file. i was thinking something like this. @peterrsongg. this function is getting really big and we are adding onto it now
GarrettBeatty
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approving since i read through https://github.com/AlexDaines/pr-4113-responseformat-demo/blob/main/Program.cs and the test program seems correct.
Can we create backlogs items for the following though.
- Setup integration tests for this project.
- Clean up this chat client class/restructure it. e.g. the get GetResponseAsync function is still super long) and i think the class should have some restructuring
Add ChatOptions.ResponseFormat support for Bedrock MEAI
Description
Implements support for
ChatOptions.ResponseFormatin the AWSSDK.Extensions.Bedrock.MEAI implementation ofIChatClient. WhenResponseFormatis set toJsonorForJsonSchema, the client now uses Bedrock's tool mechanism to enforce structured JSON responses from models.Implementation approach:
toolChoiceDocumentobjects to standard JSONWhy synthetic tool?:
Bedrock lacks a native responseFormat API; all AWS SDKs (boto3, Java, now .NET) use tool calling as the official mechanism for structured output—we inject a synthetic tool with the JSON schema to implement ChatOptions.ResponseFormat transparently.
Key behavior:
ResponseFormat.Json: Requests JSON with generic object schemaResponseFormat.ForJsonSchema: Requests JSON conforming to custom schemaResponseFormat.Text: No changes to request (default behavior)ArgumentExceptionifResponseFormatis used with user-provided tools (mutual exclusivity)NotSupportedExceptionfor streaming requests (Bedrock limitation)Motivation and Context
Closes #3911
Users need consistent behavior when using
IChatClientacross different AI providers. Currently, the Bedrock implementation ignoresChatOptions.ResponseFormat, making it impossible to request structured responses through the standardized Microsoft.Extensions.AI interface. This prevents Bedrock from being a drop-in replacement for other providers in structured data workflows.Testing
Dryrun:
.NET v4 Build: DRY_RUN-9edf05db-56d5-4398-902c-826d8573804d
Test coverage:
ResponseFormat_Json_WithSchema_CreatesSyntheticToolWithCorrectSchema: Validates synthetic tool creation with custom schemaResponseFormat_Json_ModelReturnsToolUse_ExtractsJsonCorrectly: Validates JSON extraction from tool use responsesTypes of changes
Checklist
License